Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fallible storage get and set methods for Lazy and Mapping #1910

Merged
merged 25 commits into from
Oct 27, 2023

Conversation

xermicus
Copy link
Contributor

@xermicus xermicus commented Sep 13, 2023

Adds try_* methods for reading and writing Lazy and Mapping values to and from storage (for Mapping, the encoded size of the key is also accounted for).

Fallible writes are implemented by extending the Storable trait with encoded_size method (which provides a default implementation using the SCALE version method of encoded_size). There is also size_hint in SCALE but I think we want an exact number and not an estimate. This will encode the stored value twice (the first time it's thrown away; no allocation happening), however users can optimize it by overwriting the encoded_size method to return a constant instead if possible and required.

Fallible reads are implemented using the contains_storage_key API, which will tell the size. This implies two calls to the contracts pallet (one to get the size and one to read the value). This can be improved by adding a corresponding ReturnCode to the contracts pallet, which can then be used by the get_storage API method. But for now, the two subsequent calls are necessary.

xermicus and others added 10 commits September 12, 2023 21:54
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: xermicus <cyrill@parity.io>
Signed-off-by: xermicus <cyrill@parity.io>
Signed-off-by: xermicus <cyrill@parity.io>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
@xermicus xermicus changed the title Fallible storage get and set methods for Lazy Fallible storage get and set methods for Lazy and Mapping Sep 27, 2023
@xermicus xermicus marked this pull request as ready for review September 27, 2023 15:39
@xermicus xermicus mentioned this pull request Sep 27, 2023
5 tasks
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Copy link
Collaborator

@xgreenx xgreenx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and clean to me=)

crates/ink/macro/src/storage/storable.rs Outdated Show resolved Hide resolved
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
@paritytech-cicd-pr
Copy link

paritytech-cicd-pr commented Oct 25, 2023

🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑

These are the results when building the integration-tests/* contracts from this branch with cargo-contract 4.0.0-alpha-e8c2cfc and comparing them to ink! master:

Contract Upstream Size (kB) PR Size (kB) Diff (kB) Diff (%) Change
basic-contract-caller 2.967 2.967 0 0
basic-contract-caller/other-contract 1.337 1.337 0 0
call-builder-return-value 8.688 8.735 0.047 0.540976 📈
call-runtime 1.769 1.769 0 0
conditional-compilation 1.209 1.209 0 0
contract-storage 7.124 7.171 0.047 0.659742 📈
contract-terminate 1.092 1.092 0 0
contract-transfer 1.444 1.444 0 0
custom-allocator 7.428 7.428 0 0
dns 7.249 7.249 0 0
e2e-call-runtime 1.058 1.058 0 0
e2e-runtime-only-backend 1.635 1.635 0 0
erc1155 14.026 13.962 -0.064 -0.456295 📉
erc20 6.687 6.687 0 0
erc721 9.64 9.64 0 0
events 4.763 4.763 0 0
flipper 1.393 1.393 0 0
incrementer 1.221 1.221 0 0
lang-err-integration-tests/call-builder-delegate 2.317 2.317 0 0
lang-err-integration-tests/call-builder 4.847 4.847 0 0
lang-err-integration-tests/constructors-return-value 1.773 1.773 0 0
lang-err-integration-tests/contract-ref 4.328 4.328 0 0
lang-err-integration-tests/integration-flipper 1.571 1.571 0 0
mapping-integration-tests 3.203 7.685 4.482 139.931 📈
mother 9.508 9.508 0 0
multi-contract-caller 5.924 5.924 0 0
multi-contract-caller/accumulator 1.095 1.095 0 0
multi-contract-caller/adder 1.669 1.669 0 0
multi-contract-caller/subber 1.689 1.689 0 0
multisig 21.471 21.471 0 0
payment-channel 5.501 5.501 0 0
sr25519-verification 0.865 0.865 0 0
static-buffer 1.405 1.405 0 0
trait-dyn-cross-contract-calls 2.466 2.466 0 0
trait-dyn-cross-contract-calls/contracts/incrementer 1.305 1.305 0 0
trait-erc20 7.063 7.063 0 0
trait-flipper 1.209 1.209 0 0
trait-incrementer 1.37 1.37 0 0
upgradeable-contracts/delegator 2.908 2.908 0 0
upgradeable-contracts/delegator/delegatee 1.369 1.369 0 0
upgradeable-contracts/set-code-hash 1.464 1.464 0 0
upgradeable-contracts/set-code-hash/updated-incrementer 1.443 1.443 0 0
wildcard-selector 2.622 2.622 0 0

Link to the run | Last update: Fri Oct 27 13:13:23 CEST 2023

Copy link
Collaborator

@ascjones ascjones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I'd like to see the try_ methods exercised in the mapping-integration-tests e2e tests though.

crates/ink/macro/src/tests/storable.rs Outdated Show resolved Hide resolved
crates/ink/macro/src/storage/storable.rs Outdated Show resolved Hide resolved
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Copy link
Collaborator

@ascjones ascjones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! (needs merge conflict fixing)

Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Signed-off-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
@xermicus xermicus merged commit 48ed3f8 into use-ink:master Oct 27, 2023
23 checks passed
xermicus added a commit that referenced this pull request Nov 6, 2023
Followup to #1910. The on-chain engine encodes the key and the value into the scoped buffer, so we need to account for both (same as Mapping).
@SkymanOne SkymanOne mentioned this pull request Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants